-
Notifications
You must be signed in to change notification settings - Fork 1
Fix highlight.js loading error and enhance visual elements #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR introduces a robust script loader with fallback sources and DOMContentLoaded-driven initialization to resolve highlight.js load errors, implements a custom marked renderer that handles ASCII diagrams and injects copy/execute buttons in code blocks, refactors CSS into a variable-driven, responsive layout with a dynamic sidebar and search, and adds a documentation file detailing the highlight.js fix. Sequence Diagram for Robust Script LoadingsequenceDiagram
participant B as Browser
participant LS as loadScript Function
participant CDN1 as Primary CDN
participant CDN2 as Fallback CDN
B->>LS: Call loadScript(primarySrc, fallbackSrc, callback)
LS->>CDN1: Request script from primarySrc
alt Script loads from Primary CDN
CDN1-->>LS: Script loaded successfully
LS->>B: Execute callback()
else Primary CDN fails
LS-->>CDN1: Error loading script
LS->>CDN2: Request script from fallbackSrc
alt Script loads from Fallback CDN
CDN2-->>LS: Script loaded successfully
LS->>B: Execute callback()
else Fallback CDN fails
LS-->>CDN2: Error loading script
LS->>B: Log error "Failed to load script from fallback source"
end
end
Sequence Diagram for App Initialization and Chapter LoadingsequenceDiagram
participant U as User
participant DOM as Browser DOM
participant App as JS Application
participant MSrc as Markdown Source (Cache/Fetch)
participant Marked as marked.js (with Custom Renderer)
participant HLJS as highlight.js
participant DP as DOMPurify
U->>DOM: Loads page
DOM-->>App: DOMContentLoaded event
App->>App: initializeApp(): Check library readiness (hljs, marked, DOMPurify)
opt Libraries not ready
App->>App: Wait and recheck
end
App->>App: initApp(): Setup UI (sidebar, etc.)
App->>App: loadChapter(chapterId)
App->>MSrc: Request chapter content
MSrc-->>App: Return markdown string
App->>Marked: parse(markdownString)
Marked->>HLJS: (via custom renderer) Highlight code
HLJS-->>Marked: Highlighted code fragments
Marked-->>App: Return HTML string
App->>DP: sanitize(htmlString)
DP-->>App: Return sanitized HTML
App->>DOM: Update chapter content area
App->>DOM: Update sidebar (subchapters, active links)
Class Diagram for Client-Side Application ModulesclassDiagram
class App {
+chapters: Array
+markdownCache: Object
+currentChapterId: String
+initializeApp()
+initApp()
+loadChapter(chapterId)
+updateNavigationButtons()
+updateSubchapters(chapterId, contentElement)
+initializeSidebar()
+toggleSubchapters(chapterId)
+search(query)
+addToSearchIndex(chapterId, title, content)
}
class ScriptLoader {
+loadScript(src, fallbackSrc, callback)
}
class CustomMarkedRenderer {
+code(code, language): string
+table(header, body): string
}
class UIEventListeners {
+handleSidebarToggle()
+handleDownloadPDF()
+handlePrintPage()
+handleSearchInput()
+handleKeyboardShortcuts()
+copyCode(button)
+executeCode(button)
}
class ExternalLibraries {
<<Service>> highlight.js
<<Service>> marked.js
<<Service>> DOMPurify
}
App --> ScriptLoader : uses
App --> CustomMarkedRenderer : configures marked.js with
App --> UIEventListeners : manages
CustomMarkedRenderer ..> ExternalLibraries : uses highlight.js
App ..> ExternalLibraries : uses marked.js, DOMPurify
ScriptLoader ..> ExternalLibraries : loads
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Deploy Preview for aeamcp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the documentation to explain how the highlight.js loading error was fixed and how visual elements (code blocks, ASCII diagrams, buttons) were enhanced.
- Adds robust script loading with CDNs and fallbacks
- Describes improved initialization and error handling sequences
- Documents rendering logic for code blocks, diagrams, and interactive buttons
Comments suppressed due to low confidence (1)
fixed_hljs_documentation.md:113
- The documentation references
copyCodeandexecuteCodehandlers but doesn’t specify their implementation or imports; consider adding a note or snippet showing how these functions should be defined.
html += '<button class="copy-button" onclick="copyCode(this)">Copy</button>';
| } | ||
| }; | ||
| document.head.appendChild(script); | ||
| } |
Copilot
AI
May 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a short example immediately after the loadScript function showing how to call it for each library (e.g., loadScript(primaryUrl, fallbackUrl, initializeApp);) so integrators know how to actually use this loader.
| } | |
| } | |
| // Example usage of loadScript | |
| loadScript( | |
| 'https://cdn.jsdelivr.net/npm/[email protected]/lib/highlight.min.js', // Primary URL | |
| 'https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.7.0/highlight.min.js', // Fallback URL | |
| function() { | |
| console.log('highlight.js loaded successfully'); | |
| // Initialize the app or perform actions requiring highlight.js | |
| if (typeof hljs !== 'undefined') { | |
| hljs.highlightAll(); | |
| } else { | |
| console.error('highlight.js is not available'); | |
| } | |
| } | |
| ); |
| html += ` class="language-${language}"`; | ||
| } | ||
|
|
||
| html += `>${hljs.highlightAuto(code, language ? [language] : undefined).value}</code></pre>`; |
Copilot
AI
May 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Suggest calling hljs.highlightElement (used earlier in this doc) when a specific language is known, as highlightAuto can misidentify languages and may be slower on large blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary:
This PR transforms the static documentation into an interactive dynamic system with features like:
• Dynamic chapter loading from markdown files
• Collapsible sidebar with search functionality
• Enhanced code blocks with copy/execute buttons
• Responsive design and improved styling
• Robust script loading with CDN fallbacks
Review Summary:
I identified 4 critical issues that need to be addressed before this PR can be merged. The main problems are a logic error in highlight.js usage that causes conflicts, missing markdown files that break the core functionality, a security risk in the code execution feature, and an incorrect PDF download path. I utilized my knowledge of the repository structure and JavaScript best practices to perform this review. Please provide feedback on this review which I will take into account for future reviews.
Follow-up suggestions:
• @devloai fix the identified issues in the comments
• @devloai create the missing markdown files by splitting the existing documentation
| html += ` class="language-${language}"`; | ||
| } | ||
|
|
||
| html += `>${hljs.highlightAuto(code, language ? [language] : undefined).value}</code></pre>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Logic Error in highlight.js Usage
The code is calling hljs.highlightAuto() inside the marked renderer's code function (line 724), but this creates a chicken-and-egg problem. The marked renderer is executed when marked.parse() is called, which happens before highlight.js has had a chance to process the code blocks. This will result in immediate execution of highlight.js during markdown parsing, potentially before the DOM is ready and certainly before hljs.highlightElement() is called later.
Issues:
hljs.highlightAuto()returns an object with a.valueproperty, but it's being used as if it returns a string- This approach bypasses the normal highlighting flow and may cause conflicts
- The code blocks will be highlighted twice - once during markdown parsing and once during DOM processing
Solution:
| html += `>${hljs.highlightAuto(code, language ? [language] : undefined).value}</code></pre>`; | |
| html += `>${code}</code></pre>`; |
Remove the hljs.highlightAuto() call here and let the normal highlighting process in hljs.highlightElement() handle syntax highlighting after the DOM is ready.
| { id: "11", title: "Future Directions", file: "research_draft_part11.md" }, | ||
| { id: "12", title: "Conclusion", file: "research_draft_part12.md" }, | ||
| { id: "13", title: "References", file: "research_draft_part13.md" }, | ||
| { id: "14", title: "Appendices", file: "research_draft_part14.md" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded Chapter Configuration Doesn't Match Actual Content
The application is hardcoded to load 14 specific markdown files (research_draft_part1.md through research_draft_part14.md), but based on the repository structure, these files don't exist. The actual repository contains:
index.html(this file)draft.rs(Rust smart contract)README.md(project documentation)aeamcp.pdfpreview.pngfixed_hljs_documentation.md
Problems:
- All chapter loading will fail with 404 errors
- The search functionality won't work as there's no content to index
- Users will see error messages for missing files
Solution:
Either:
- Create the expected markdown files by splitting the existing documentation
- Or modify the chapter configuration to use existing content like
README.mdanddraft.rs - Or implement a fallback to load content from the existing static documentation structure
| // This is a simplified approach - in a real environment, | ||
| // you would need to use a more sophisticated method to open a terminal | ||
| const terminal = window.open('', '_blank'); | ||
| if (terminal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Risk: Arbitrary Code Execution via Popup
The executeCode function opens a new window and writes arbitrary content to it using document.write(). This creates several security concerns:
Issues:
- XSS Risk: If code contains malicious HTML/JavaScript, it will be executed in the new window
- Popup Blockers: Modern browsers will likely block this popup
- User Experience: Opening random popups is poor UX and may be flagged as malicious
Solution:
function executeCode(button) {
const pre = button.closest('pre');
const code = pre.querySelector('code');
const command = code.textContent.trim();
// Safely copy to clipboard and show instructions
navigator.clipboard.writeText(command).then(() => {
alert(`Command copied to clipboard:\n\n${command}\n\nPlease paste and execute in your terminal.`);
}).catch(err => {
// Fallback for unsupported browsers
alert(`Please copy and execute this command in your terminal:\n\n${command}`);
});
}This approach is safer and provides better user experience.
| // Download PDF | ||
| downloadPDFButton.addEventListener('click', () => { | ||
| // Open PDF in new tab | ||
| window.open('solana_protocol_design_for_agent_and_mcp_server_registries.pdf', '_blank'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect PDF Download Path
The download PDF functionality references 'solana_protocol_design_for_agent_and_mcp_server_registries.pdf' but the actual PDF file in the repository is named aeamcp.pdf.
Problem:
- Users will get a 404 error when trying to download the PDF
- The functionality will appear broken
Solution:
| window.open('solana_protocol_design_for_agent_and_mcp_server_registries.pdf', '_blank'); | |
| window.open('aeamcp.pdf', '_blank'); |
Update the path to match the actual file in the repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @0xrinegade - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <!-- Load libraries in the correct order with fallbacks --> | ||
| <script> | ||
| // Function to load script with fallback | ||
| function loadScript(src, fallbackSrc, callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Handle permanent load failures to prevent infinite retry loop
Add a maximum retry limit or an abort flag to stop retries when both sources fail.
| code = typeof code === 'string' ? code : String(code); | ||
|
|
||
| // Check if this is an ASCII diagram | ||
| const isAsciiDiagram = code.includes('+-') || code.includes('|') || code.includes('--') || code.includes('==') || code.includes('[]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): ASCII diagram detection may misclassify generic code
Consider using an explicit language marker (e.g., ```ascii) or a front-matter flag to identify diagrams, rather than relying on character patterns that may misclassify regular code.
| currentChapterId = chapterId; | ||
| } | ||
|
|
||
| // Update active state in sidebar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Avoid changing 'active' link when only toggling subchapter list
Move the 'active' class update inside the content loading logic or separate the toggle and content update logic to keep the sidebar state consistent with the displayed chapter.
| </head> | ||
| <body> | ||
| <div class="container"> | ||
| <button id="sidebarToggle" class="sidebar-toggle">☰</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Add ARIA attributes to sidebar toggle for accessibility
Add aria-expanded, aria-controls, and an aria-label to the toggle button to improve screen reader accessibility.
| <button id="sidebarToggle" class="sidebar-toggle">☰</button> | |
| <button id="sidebarToggle" class="sidebar-toggle" aria-expanded="false" aria-controls="sidebar" aria-label="Toggle sidebar">☰</button> |
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Use semantic tags (e.g.
, ) instead of genericWrap the main content in a
element and use for navigation to improve accessibility and structure.Suggested implementation:
<body>
<header>
<button id="sidebarToggle" class="sidebar-toggle">☰</button>
<!-- Place navigation or branding here if needed -->
</header>
<main class="container">
<!-- External libraries with fallback mechanisms -->
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.8.0/styles/github-gist.min.css">
<!-- Load libraries in the correct order with fallbacks -->
<script>
// Function to load script with fallback
function loadScript(src, fallbackSrc, callback) {
var script = document.createElement('script');
script.src = src;
script.onload = callback;
You will need to:
- Move any other main content that was inside the
.containerdiv into the new<main class="container">element. - Ensure that the
<main>and<header>tags are properly closed at the appropriate places in the file. - If there is navigation or branding, move it into the
<header>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Summary by Sourcery
Fix highlight.js loading errors by adding a resilient script loader with fallbacks and reordering initialization; overhaul page structure and styling with CSS variables, a collapsible sidebar, and responsive design; introduce interactive features such as live search, chapter navigation, code copy/execute buttons, and PDF/print controls.
New Features:
Bug Fixes:
Enhancements:
Documentation: